lib.network: add ipv6 parser #318712

woojiq · 2024-06-10T06:34:18Z

Description of changes

Add a library function to parse and validate an IPv6 address from a
string. It can parse the first two versions of an IPv6 address according
to https://datatracker.ietf.org/doc/html/rfc4291#section-2.2. The third
form "x:x:x:x:x:x.d.d.d.d" is not yet implemented.
Add shell script network.sh to test IPv6 parser functionality.
Copy fromHexString function from the lib: add fromHexString #266705

Mentor: @Janik-Haag

Things done

Tested, as applicable:

NixOS test(s) (look inside nixos/tests)
and/or package tests
or, for functions and "core" functionality, tests in lib/tests or pkgs/test

Fits CONTRIBUTING.md.
Formatted using nixfmt-rfc-style

Add a 👍 reaction to pull requests you find important.

lib/network/default.nix

mweinelt · 2024-06-17T14:40:55Z

lib/network/default.nix

+      in
+      assert lib.asserts.assertMsg (
+        builtins.length addr == 8
+      ) "_parseNormalized expected to recieve normalized IPv6 address";


Suggested change

) "_parseNormalized expected to recieve normalized IPv6 address";

) "_parseNormalized expects a normalized IPv6 address";

But what does "normalized" in this case even mean? I don't think the function name matches its purpose well.

lib/network/default.nix

.gitignore

lib/trivial.nix

lib/tests/network.sh

lib/network/default.nix

Janik-Haag · 2024-06-18T20:32:24Z

To address your questions:

Do I need to somehow hide the internal functions more than just underscoring them?

Yes currently they would all get exposed, you have to put them into a let binding to make them truly internal. Take a look at https://github.com/NixOS/nixpkgs/blob/master/lib/fileset/default.nix#L103 as an example.

Are the doc comments in the correct format (already existing files have different style)?

Some of them are some of them aren't, please take a look at the example

Do I need to pull the ipv4 parser implementation to merge them together? #299409

no, we should keep them separate and preferably go for small prs, e.g. once the parser is fine merge this pr and then do another v6 one for cidr related functions.

woojiq · 2024-06-22T12:21:16Z

@Janik-Haag @mweinelt I refactored the code and moved internal functions to internal.nix. Can you review again please?

Janik-Haag

I think this is getting into shape. There are some smaller points left to do, and you should add a lot more comments explaining different parts, I think. This part of lib currently is very undocumented internally compared to others like lib.fileset or lib.generators.

lib/network/internal.nix

Janik-Haag · 2024-06-26T20:13:39Z

lib/network/internal.nix

+            if 1 <= n && n <= ipv6Bits then
+              n
+            else
+              throw "${apiName}: IPv6 subnet should be in range [1;${toString ipv6Bits}], got ${toString n}";


Uh, I don't think we want to pass around the function name here, this will only get more confusing to debug because your output would gaslight the user, once other functions rely on the ${apiName} function internally.

Janik-Haag · 2024-06-26T20:15:20Z

lib/network/default.nix

+        api = "lib.network.ipv6.fromString";
+
+        splittedAddr = _ipv6.split api addr;


Please remove the "api" part.

Janik-Haag · 2024-06-26T20:32:47Z

lib/network/internal.nix

+        emptyExpected =
+          if hasPrefix || hasSuffix then
+            2
+          else if hasInfix then
+            1
+          else
+            0;


This should be moved in the same scope as the emptyCount definition

Janik-Haag · 2024-06-26T21:20:54Z

lib/network/internal.nix

+  ipv6Bits = 128;
+  ipv6Pieces = 8;
+  ipv6PieceBits = 16;


We should link to the corresponding RFC definitions, and maybe write a comment explaining them, since these are magic numbers to anyone unfamiliar with networking.

Janik-Haag · 2024-06-26T21:30:09Z

lib/network/internal.nix

+        let
+          zeros = genList (_: "0") (ipv6Pieces - piecesNoEmptyLen);
+        in
+        if hasPrefix then zeros ++ piecesNoEmpty else piecesNoEmpty ++ zeros
+      else if hasInfix then
+        concatMap (
+          piece: if piece == "" then genList (_: "0") (ipv6Pieces - piecesNoEmptyLen) else [ piece ]
+        ) pieces


We can reuse the zeros variable in line 68 in the concatMap function, because it's doing the same.

Janik-Haag · 2024-06-26T21:43:55Z

lib/network/internal.nix

+      let
+        splitted = strings.splitString "/" addr;
+        splittedLength = length splitted;
+      in
+      if splittedLength == 1 then # [ ip ]
+        {
+          address = parseIpv6FromString apiName addr;
+          prefixLength = 64; # /64 is a standard size IPv6 subnet as defined by the IETF.
+        }
+      else if splittedLength == 2 then # [ ip subnet ]


I think we should be stricter with the user input and don't silently just set some prefix but instead fail saying that it's not a cidr address.
Also, usually the assumption I'm familiar with if you just don't specify a prefix is that you get a /128.

I agree, 128 sounds more logical if prefix length is omitted.

I think we should be stricter with the user input and don't silently just set some prefix but instead fail saying that it's not a cidr address.

I added one fromString function and user can use both default and cidr notation. I don't see a reason to create two separate API functions like fromString and fromCidrString, because this can be easily observed from the passed string - if it contains a /, then we try to parse it as cidr notation.

Co-authored-by: lucasew <lucas59356@gmail.com>

Add a library function to parse and validate an IPv6 address from a string. It can parse the first two versions of an IPv6 address according to https://datatracker.ietf.org/doc/html/rfc4291#section-2.2. The third form "x:x:x:x:x:x.d.d.d.d" is not yet implemented. Optionally parser can accept prefix length (128 is default). Add shell script network.sh to test IPv6 parser functionality.

Janik-Haag

@infinisil as a lib code owner, do you want to give this a review before we merge it?

lib/network/default.nix

nixos-discourse · 2024-07-11T18:40:08Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/showcase-nixpkgs-network-library/48897/1

github-actions bot added the 6.topic: lib The Nixpkgs function library label Jun 10, 2024

woojiq force-pushed the lib-network-ipv6-parser branch from d2bceb3 to 69089c5 Compare June 10, 2024 07:31

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jun 10, 2024

mweinelt reviewed Jun 17, 2024

View reviewed changes

Janik-Haag reviewed Jun 18, 2024

View reviewed changes

woojiq force-pushed the lib-network-ipv6-parser branch 2 times, most recently from 8ad8940 to 5f1b84c Compare June 22, 2024 11:53

woojiq marked this pull request as ready for review June 22, 2024 11:58

woojiq requested a review from infinisil as a code owner June 22, 2024 11:58

woojiq marked this pull request as draft June 22, 2024 12:00

woojiq force-pushed the lib-network-ipv6-parser branch from 5f1b84c to 05c8c7b Compare June 22, 2024 12:19

woojiq marked this pull request as ready for review June 22, 2024 12:20

woojiq marked this pull request as draft June 23, 2024 08:36

woojiq force-pushed the lib-network-ipv6-parser branch from 05c8c7b to 1ed35a9 Compare June 23, 2024 17:06

woojiq marked this pull request as ready for review June 23, 2024 17:08

woojiq mentioned this pull request Jun 23, 2024

lib.network: first/last and next addresses in a network #322004

Draft

3 tasks

Janik-Haag requested changes Jun 26, 2024

View reviewed changes

woojiq force-pushed the lib-network-ipv6-parser branch from 1ed35a9 to 504a79b Compare June 28, 2024 17:40

woojiq and others added 2 commits June 28, 2024 20:53

gitignore: add .helix config directory

9cd5b7a

lib: add fromHexString

52cc703

Co-authored-by: lucasew <lucas59356@gmail.com>

woojiq force-pushed the lib-network-ipv6-parser branch from 504a79b to 9a035ac Compare June 28, 2024 17:54

woojiq force-pushed the lib-network-ipv6-parser branch from 9a035ac to d559eed Compare June 29, 2024 07:02

Janik-Haag reviewed Jul 7, 2024

View reviewed changes

lib/network/default.nix Show resolved Hide resolved

Janik-Haag approved these changes Jul 11, 2024

View reviewed changes

Janik-Haag merged commit c20399e into NixOS:master Jul 11, 2024
21 checks passed

woojiq deleted the lib-network-ipv6-parser branch July 11, 2024 19:00

woojiq mentioned this pull request Jul 13, 2024

lib: add fromHexString #266705

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib.network: add ipv6 parser #318712

lib.network: add ipv6 parser #318712

woojiq commented Jun 10, 2024 •

edited

Loading

mweinelt Jun 17, 2024

Janik-Haag commented Jun 18, 2024

woojiq commented Jun 22, 2024

Janik-Haag left a comment

Janik-Haag Jun 26, 2024

Janik-Haag Jun 26, 2024

Janik-Haag Jun 26, 2024

Janik-Haag Jun 26, 2024

Janik-Haag Jun 26, 2024

Janik-Haag Jun 26, 2024

woojiq Jun 28, 2024 •

edited

Loading

Janik-Haag left a comment

nixos-discourse commented Jul 11, 2024

	) "_parseNormalized expected to recieve normalized IPv6 address";
	) "_parseNormalized expects a normalized IPv6 address";

		api = "lib.network.ipv6.fromString";

		splittedAddr = _ipv6.split api addr;

lib.network: add ipv6 parser #318712

lib.network: add ipv6 parser #318712

Conversation

woojiq commented Jun 10, 2024 • edited Loading

Description of changes

Things done

mweinelt Jun 17, 2024

Choose a reason for hiding this comment

Janik-Haag commented Jun 18, 2024

woojiq commented Jun 22, 2024

Janik-Haag left a comment

Choose a reason for hiding this comment

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

woojiq Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Janik-Haag left a comment

Choose a reason for hiding this comment

nixos-discourse commented Jul 11, 2024

woojiq commented Jun 10, 2024 •

edited

Loading

woojiq Jun 28, 2024 •

edited

Loading